adding custom changes to grammes fork #1

waleedakramkhan · 2023-01-03T16:34:00Z

https://k6scope.atlassian.net/browse/BLUEP-175

This PR adds custom changes made in https://gitlab.com/kaleidoscope.root/grammes-gremlin to this fork

waleedakramkhan · 2023-01-03T16:35:05Z

i've also added test for a custom StringStep functionality added by @MDanialSaleem please review

waleedakramkhan

Our version is too old - even older than K6scope itself i believe (at least more than 4 years) i have reviewed and commented on what i believe how the two versions should be merged but i lack the context for these changes so im importing them and leaving them here for all stakeholders to decide unanimously around these. my opinions are in the comments.

my preference would be to not introduce so many changes because from what i have seen, original library provides all methods to attain the goals we are with our custom changes except for type ones (maybe those too but not able to comment confidently).

waleedakramkhan · 2023-01-04T13:15:34Z

gremconnect/response.go


-	err = responseDetectError(resp.Code, message)


Changes in this file were removed here

should be put back imo

waleedakramkhan · 2023-01-04T13:17:00Z

gremconnect/response_test.go

@@ -77,7 +77,6 @@ var (
 {
    "requestId": "d2476e5b-b2bc-6a70-2647-3991f68ab415",
    "status": {
-        "message": "Some error",


Changes in this file were removed here

should be put back imo

waleedakramkhan · 2023-01-04T13:17:32Z

gremerror/networkerror.go

@@ -26,15 +26,13 @@ import "strconv"
 type NetworkError struct {
 	statusCode int
 	msg        string
-	origMsg    string


Changes in this file were removed here

should be put back imo

manager/addvertex.go

waleedakramkhan · 2023-01-04T13:19:09Z

manager/misc.go

@@ -21,8 +21,6 @@
 package manager

 import (
-	"errors"
-	"fmt"


don't think this change will be accepted by maintainers of this repo

waleedakramkhan · 2023-01-04T13:22:11Z

query/traversal/addproperty.go

@@ -32,11 +38,42 @@ package traversal
 // Property(interface{} (Object), interface{} (Object), ...interface{} (Object))
 // Property(Cardinality, string (Object), interface{} (Object), ...interface{} (Object))
 func (g String) Property(objOrCard interface{}, obj interface{}, params ...interface{}) String {
-	fullParams := make([]interface{}, 0, len(params)+2)


i believe that this change should be reverted..it's already handled by library in a much better design after refactoring..

query/traversal/graph.go

waleedakramkhan · 2023-01-04T13:23:31Z

query/traversal/stringstep.go

@@ -0,0 +1,14 @@
+package traversal
+
+// StringStep adds a custom string to the traversal. This is not a gremlin step.


added by @MDanialSaleem will add tests for it

waleedakramkhan · 2023-01-04T13:24:00Z

query/traversal/stringstep_test.go

+	. "github.com/smartystreets/goconvey/convey"
+)
+
+func TestStringStep(t *testing.T) {


tests for functionality added by @MDanialSaleem

waleedakramkhan · 2023-01-04T13:25:10Z

query/traversal/util.go

@@ -88,6 +88,10 @@ func (g *String) AddStep(step string, params ...interface{}) {
 			g.buffer.Write(t)
 		case string:
 			g.buffer.WriteString("\"" + strings.ReplaceAll(t, "\"", "\\\"") + "\"")
+		case int64, uint64:


moved the change in refactored newer version here

kingzbauer

Left a couple of comments

manager/addvertex.go

model/vertex_query.go

kingzbauer · 2023-01-13T11:21:35Z

model/vertex_test.go

+			edge1 := EdgeWithPropsAndLabel{
+				Label:      "testLbl",
+				Id:         tstoutVID,
+				Properties: []interface{}{"tstIntrf1", 7777, "tstIntrf2", 9876},


We could probably include a set property with a value of type e.g []int or []string and []interface{} to make sure they are handled correctly.

Properties are a list of interface and we cannot send int as interface, can we?

kingzbauer · 2023-01-13T11:36:12Z

query/traversal/addproperty.go

+
+	if len(params) > 0 {
+		for _, p := range params {
+			switch p.(type) {


Should we explicitly check for float as well?

MDanialSaleem · 2023-01-13T15:54:00Z

gremconnect/websocket.go

-		WriteBufferSize:  1024 * 8, // Set up for large messages.
-		ReadBufferSize:   1024 * 8, // Set up for large messages.
+		WriteBufferSize:  1024 * 256, // Set up for large messages.
+		ReadBufferSize:   1024 * 256, // Set up for large messages.


we should make this configurable.

Should we add another method that takes this as param as input? Otherwise we'll have to modify this signature which might not work for upstream.

I think for now, we can let it be. There needs to be a more detailed investigation into why we changed what we changed, and if there is a better way to make those changes, before we can merge this to upstream.

i tried this but this needs more parameterizing than it looks so i reverted my changes. i think it's better to open up this issue on grammes and then see how it goes

MDanialSaleem · 2023-03-01T04:32:38Z

go.mod

 	go.uber.org/zap v1.9.1
 )
+
+replace github.com/northwesternmutual/grammes v1.2.0 => github.com/Kaleidoscope-Inc/grammes v1.2.1-0.20230221065140-059981b86c6f


why does this package attempts to refer to an older version of itself?

this was an attempt to not change module everywhere since we want it to use our changes but it failed because go has problems with moving on from her ex - i decided to create a separate branch and use this in our code

Oh I see that makes sense. That probably has to do with go's package mechanism + forking. Anyway, LGTM!

MDanialSaleem

one minor comment otherwise lgtm

adding custom changes to grammes fork

70ad095

waleedakramkhan requested review from kingzbauer, MDanialSaleem and zubair-nabi January 3, 2023 16:34

waleedakramkhan self-assigned this Jan 3, 2023

waleedakramkhan marked this pull request as draft January 3, 2023 18:07

Waleed Akram Khan added 2 commits January 3, 2023 23:53

took a diff with grammes in gitlab and merged all changes

d471d49

missed one

2dce598

waleedakramkhan commented Jan 4, 2023

View reviewed changes

waleedakramkhan marked this pull request as ready for review January 4, 2023 13:32

Waleed Akram Khan and others added 8 commits January 5, 2023 12:04

changing from zap int64 to zap any

f638a13

some more zaps

4cd0f1b

whitespaces

367eec7

Removing superflous files from PR.

3bc5d2b

Removing another superflous file.

e41aa73

Syncing with upstream.

bc9bda3

Adding test for addEdges.

cab68bb

Adding tests for SetList and SetProperty.

9031cc2

kingzbauer reviewed Jan 13, 2023

View reviewed changes

MDanialSaleem reviewed Jan 13, 2023

View reviewed changes

Waleed Akram Khan and others added 5 commits January 22, 2023 19:17

removing some changes

f792e43

replacing modules

059981b

updating module

8767e62

adding custom connect

9951ce0

removing custom connect

57559b8

MDanialSaleem reviewed Mar 1, 2023

View reviewed changes

MDanialSaleem approved these changes Mar 1, 2023

View reviewed changes

MDanialSaleem approved these changes Mar 2, 2023

View reviewed changes

waleedakramkhan closed this Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding custom changes to grammes fork #1

adding custom changes to grammes fork #1

waleedakramkhan commented Jan 3, 2023

waleedakramkhan commented Jan 3, 2023

waleedakramkhan left a comment

waleedakramkhan Jan 4, 2023

waleedakramkhan Jan 4, 2023

waleedakramkhan Jan 4, 2023

waleedakramkhan Jan 4, 2023

waleedakramkhan Jan 4, 2023

waleedakramkhan Jan 4, 2023

waleedakramkhan Jan 4, 2023

waleedakramkhan Jan 4, 2023

kingzbauer left a comment

kingzbauer Jan 13, 2023

waleedakramkhan Feb 20, 2023

kingzbauer Jan 13, 2023

MDanialSaleem Jan 13, 2023 •

edited

Loading

zubair-nabi Feb 21, 2023

MDanialSaleem Mar 1, 2023

waleedakramkhan Mar 2, 2023

MDanialSaleem Mar 1, 2023

waleedakramkhan Mar 2, 2023

MDanialSaleem Mar 2, 2023

MDanialSaleem left a comment

		@@ -0,0 +1,14 @@
		package traversal

		// StringStep adds a custom string to the traversal. This is not a gremlin step.

adding custom changes to grammes fork #1

adding custom changes to grammes fork #1

Conversation

waleedakramkhan commented Jan 3, 2023

waleedakramkhan commented Jan 3, 2023

waleedakramkhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingzbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MDanialSaleem Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MDanialSaleem left a comment

Choose a reason for hiding this comment

MDanialSaleem Jan 13, 2023 •

edited

Loading